Skip to content

Conversation

@greenc-FNAL
Copy link
Contributor

@greenc-FNAL greenc-FNAL commented Dec 19, 2025

Goal

The primary goal of this PR is to significantly improve the test coverage for the Phlex Python plugin and resolve several discovered issues related to memory management, type conversion, and core graph connectivity safety. It also modernizes the build system configuration and coverage tooling.

Key Changes and Rationale

1. Introduction of phlex.Variant Helper

  • Changes: Added a new Python class Variant in plugins/python/python/phlex/__init__.py.
  • Motivation: Python algorithms are often generic, but Phlex registration requires specific C++ signatures. Additionally, Python parameter names in a function's __annotations__ might not match the positional labels defined in the Phlex configuration.
  • Solution: The Variant class allows developers to wrap a callable and associate it with custom annotations and a specific name. This enables the same Python function to be registered multiple times with different types without modifying the original function.
  • Rationale: This decouples the algorithm implementation from the registration metadata. The C++ bridge was also updated to "unwrap" these objects, retrieving the underlying phlex_callable for performance while still using the provided annotations for type mapping.

2. Robustness Improvements in modulewrap.cpp

  • Reference Counting: Added missing Py_DECREF calls in the error paths of parse_args, md_transform, and md_observe. This prevents Python object leaks when registration fails.
  • Sequence Handling: Replaced manual sequence checks with PySequence_Fast. This allows Phlex to accept any Python sequence (lists, tuples, etc.) for input/output labels.
  • Type Matching & NumPy Fixes:
    • Corrected a typo where double-precision NumPy arrays were matched against double64]] instead of float64]].
    • Updated type matching logic to use fixed-length substring comparisons, supporting PEP 604 union types (e.g., ndarray | list).
  • List-based Type Converters: Added explicit support for list[int], list[float], and list[double].
    • Rationale: Provides a fallback for users not using NumPy or for cases where standard Python lists are more appropriate.

3. Core Connectivity & Safety

  • Changes: Added null-pointer checks in phlex/core/edge_maker.cpp and phlex/core/edge_maker.hpp.
  • Motivation: Misconfigurations in the Jsonnet graph could lead to null ports during edge creation, resulting in segmentation faults.
  • Solution: The code now throws a std::runtime_error with a descriptive message identifying the offending nodes/ports.

4. Build System & Tooling Modernization

  • CMake Optimizations:
    • Introduced DART_TESTING_TIMEOUT and CTEST_TEST_TIMEOUT to prevent long stalls in CI environments.
    • Refined GCC warning suppressions for versions 14+ to reduce noise from false-positive array-bounds and string-overflow warnings.
    • Standardized Sanatizer (ASan/TSan) compile options for better compatibility.
  • Coverage Tooling: Updated scripts/coverage.sh and its documentation to support both Clang (LLVM source-based) and GCC (gcov) presets, enabling both high-fidelity local reports and CI-compatible XML outputs.
  • Documentation: Added plugins/python/README.md (architecture guide) and .github/copilot-instructions.md (project context for AI assistants).

New Tests

This PR adds a comprehensive suite of tests in test/python/ to ensure long-term stability:

  • unit_test_variant.py: Dedicated unit tests for the Variant class logic.
  • test_types.py & test_coverage.py: Exercise various scalar, vector, and list-based type conversions to ensure data integrity.
  • test_callbacks.py: Targets edge cases in modulewrap.cpp, such as 3-argument callbacks and Python exception propagation.
  • Error Path Verification: Added several tests (pybadbool.jsonnet, pybadint.jsonnet, pymismatch.jsonnet, etc.) that verify Phlex correctly catches and reports type and signature mismatches.
  • Coverage Integration: Updated test/python/CMakeLists.txt to support pytest-cov, enabling integrated Python coverage reporting.

@greenc-FNAL
Copy link
Contributor Author

@phlexbot format

@github-actions
Copy link
Contributor

Automatic cmake-format fixes pushed (commit e60d811).
⚠️ Note: Some issues may require manual review and fixing.

@greenc-FNAL
Copy link
Contributor Author

@phlexbot format

@github-actions
Copy link
Contributor

No automatic cmake-format fixes were necessary.

@github-actions
Copy link
Contributor

Automatic clang-format fixes pushed (commit 2aa7957).
⚠️ Note: Some issues may require manual review and fixing.

@greenc-FNAL
Copy link
Contributor Author

@phlexbot python-fix

@github-actions
Copy link
Contributor

Automatic Python linting fixes pushed (commit 092dec4).
⚠️ Note: Some issues may require manual review and fixing.

@codecov
Copy link

codecov bot commented Dec 19, 2025

Codecov Report

❌ Patch coverage is 73.68421% with 60 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
plugins/python/src/modulewrap.cpp 76.14% 25 Missing and 27 partials ⚠️
phlex/core/edge_maker.hpp 0.00% 3 Missing and 2 partials ⚠️
phlex/core/edge_maker.cpp 0.00% 2 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (73.68%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

@@            Coverage Diff             @@
##             main     #213      +/-   ##
==========================================
+ Coverage   75.69%   80.12%   +4.42%     
==========================================
  Files         125      125              
  Lines        2946     3074     +128     
  Branches      519      546      +27     
==========================================
+ Hits         2230     2463     +233     
+ Misses        497      383     -114     
- Partials      219      228       +9     
Flag Coverage Δ
unittests 80.12% <73.68%> (+4.42%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
plugins/python/src/configwrap.cpp 76.62% <100.00%> (ø)
plugins/python/src/lifelinewrap.cpp 53.33% <100.00%> (+3.33%) ⬆️
phlex/core/edge_maker.cpp 68.00% <0.00%> (-9.28%) ⬇️
phlex/core/edge_maker.hpp 83.33% <0.00%> (-16.67%) ⬇️
plugins/python/src/modulewrap.cpp 75.04% <76.14%> (+29.00%) ⬆️

... and 4 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cbe714...72cb86c. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@greenc-FNAL greenc-FNAL force-pushed the maintenance/improve-test-coverage branch from 38fc738 to 47661fa Compare December 19, 2025 17:19
@greenc-FNAL
Copy link
Contributor Author

@phlexbot format

@github-actions
Copy link
Contributor

No automatic clang-format fixes were necessary.

@github-actions
Copy link
Contributor

Automatic cmake-format fixes pushed (commit bb954c8).
⚠️ Note: Some issues may require manual review and fixing.

@greenc-FNAL
Copy link
Contributor Author

@phlexbot python-fix

@github-actions
Copy link
Contributor

Automatic Python linting fixes pushed (commit cfdb09a).
⚠️ Note: Some issues may require manual review and fixing.

@greenc-FNAL greenc-FNAL force-pushed the maintenance/improve-test-coverage branch 3 times, most recently from c9ccf06 to 1159796 Compare December 21, 2025 17:30
@greenc-FNAL
Copy link
Contributor Author

@phlexbot format

@github-actions
Copy link
Contributor

Automatic clang-format fixes pushed (commit b145a22).
⚠️ Note: Some issues may require manual review and fixing.

@greenc-FNAL
Copy link
Contributor Author

@phlexbot format

@github-actions
Copy link
Contributor

Automatic cmake-format fixes pushed (commit cc928e7).
⚠️ Note: Some issues may require manual review and fixing.

@github-actions
Copy link
Contributor

No automatic clang-format fixes were necessary.

@greenc-FNAL greenc-FNAL force-pushed the maintenance/improve-test-coverage branch from 36bd016 to e1d2b41 Compare February 10, 2026 22:51
@greenc-FNAL
Copy link
Contributor Author

@phlexbot clang-fix

@github-actions
Copy link
Contributor

Automatic clang-format fixes pushed (commit 81bff12).
⚠️ Note: Some issues may require manual review and fixing.

@greenc-FNAL
Copy link
Contributor Author

Review the full CodeQL report for details.

1 similar comment
@greenc-FNAL
Copy link
Contributor Author

Review the full CodeQL report for details.

@greenc-FNAL
Copy link
Contributor Author

@phlexbot clang-fix

@github-actions
Copy link
Contributor

Automatic clang-format fixes pushed (commit 281a74f).
⚠️ Note: Some issues may require manual review and fixing.

@greenc-FNAL greenc-FNAL requested a review from wlav February 10, 2026 23:01
@greenc-FNAL
Copy link
Contributor Author

Review the full CodeQL report for details.

1 similar comment
@greenc-FNAL
Copy link
Contributor Author

Review the full CodeQL report for details.

@greenc-FNAL
Copy link
Contributor Author

@phlexbot clang-fix

@github-actions
Copy link
Contributor

No automatic clang-format fixes were necessary.

@greenc-FNAL
Copy link
Contributor Author

Review the full CodeQL report for details.

wlav
wlav previously approved these changes Feb 10, 2026
Copy link
Contributor

@wlav wlav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've given all the relevant feedback I can at this point. :)

@greenc-FNAL
Copy link
Contributor Author

@phlexbot clang-fix

@github-actions
Copy link
Contributor

Automatic clang-format fixes pushed (commit 72cb86c).
⚠️ Note: Some issues may require manual review and fixing.

@greenc-FNAL
Copy link
Contributor Author

Review the full CodeQL report for details.

2 similar comments
@greenc-FNAL
Copy link
Contributor Author

Review the full CodeQL report for details.

@greenc-FNAL
Copy link
Contributor Author

Review the full CodeQL report for details.

@greenc-FNAL greenc-FNAL requested a review from wlav February 11, 2026 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants